Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixes #1188, fixes #1201 #1210

Merged
merged 11 commits into from
Mar 6, 2018
Merged

fixes #1188, fixes #1201 #1210

merged 11 commits into from
Mar 6, 2018

Conversation

ekhaled
Copy link
Contributor

@ekhaled ekhaled commented Mar 6, 2018

My first foray into svelte internals, so please forgive any stupidities.

I set out to fix #1188 , but managed to fix #1201 for free.

I have updated teh snapshots. However, I couldn't find any tests for transitions.

Let me know if I need to change anything

@codecov-io
Copy link

codecov-io commented Mar 6, 2018

Codecov Report

Merging #1210 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1210      +/-   ##
==========================================
- Coverage   91.64%   91.63%   -0.01%     
==========================================
  Files         127      127              
  Lines        4558     4557       -1     
  Branches     1503     1502       -1     
==========================================
- Hits         4177     4176       -1     
  Misses        159      159              
  Partials      222      222
Impacted Files Coverage Δ
src/generators/dom/index.ts 96.21% <ø> (-0.03%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 18d3313...f9d606a. Read the comment docs.

@ekhaled
Copy link
Contributor Author

ekhaled commented Mar 6, 2018

Found the transition tests 🤦‍♂️

@ekhaled
Copy link
Contributor Author

ekhaled commented Mar 6, 2018

Can't figure out why Travis complains. It should be able to find ./Child.html

@Rich-Harris
Copy link
Member

What do you think about putting the || null inside the _mount method, rather than in calls to _mount? That way it only appears once per codebase (I'm not totally sure it's necessary at all, now that I think of it — though it might be in some browsers...)

Slightly orthogonal to the task at hand, but seems like as good a time as any to make that change now that we're introducing more calls to _mount.

@ekhaled
Copy link
Contributor Author

ekhaled commented Mar 6, 2018

wouldn't sending options.anchor without the || throw an undefined error if options.anchor is missing?

@Rich-Harris
Copy link
Member

I meant that src/shared/index.ts would have this:

export function _mount(target, anchor) {
	this._fragment.m(target, anchor || null);
}

...and then the generated code could do this._mount(options.target, options.anchor) instead of needing the || null there

@ekhaled
Copy link
Contributor Author

ekhaled commented Mar 6, 2018

Done as suggested.

Sorry about the messy commit structure.. maybe squash and merge when you do.

While we are here,
I think we need a ternary like this in _unmount too. Because nothing in the codebase ever calls _fragment.o().
Components never transition out

@Rich-Harris
Copy link
Member

Nice, thanks. Not sure what's going on with that test failure — could just be an innocent timeout, so have restarted.

Components never transition out

Yeah, that's kind of a tricky problem — one that we can kick down the road a bit I think

@ekhaled
Copy link
Contributor Author

ekhaled commented Mar 6, 2018

please wait before you merge
There is a case sensitivity problem still. let me fix that

@ekhaled
Copy link
Contributor Author

ekhaled commented Mar 6, 2018

ok.. looks ready to merge now 😥

Yeah, that's kind of a tricky problem — one that we can kick down the road a bit I think

wouldn't something like this in _unmount fix the problem:

if(this._fragment.o){
  this._fragment.o(() => this._fragment.u())
}else{
  this._fragment.u()
}

@Rich-Harris Rich-Harris merged commit 6ea1612 into sveltejs:master Mar 6, 2018
@Rich-Harris Rich-Harris mentioned this pull request Mar 6, 2018
@Rich-Harris
Copy link
Member

Thanks so much! Released 1.56.3.

Yeah, it might actually be as simple as that. I remember thinking there was a tricky coordination problem because the transitions are happening across 'levels' (i.e. there might be some fun stuff to take into account if you need to re-render the component while it's outroing) but I might be wrong about that. Opened #1211 anyway

@ekhaled ekhaled deleted the some_fixes branch March 6, 2018 23:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

always use _mount, even if top-level Transitions inside components
3 participants